Moving soil model to new configuration#1109
Conversation
…ove-soils-over-to-the-new-configuration-system
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1109 +/- ##
===========================================
- Coverage 94.64% 94.63% -0.01%
===========================================
Files 90 90
Lines 7896 7887 -9
===========================================
- Hits 7473 7464 -9
Misses 423 423 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| turnover_rate = 2.4e-2 | ||
| c_n_ratio = 6.5 | ||
| c_p_ratio = 40.0 | ||
| """, |
There was a problem hiding this comment.
Very happy for this monstrosity to die
| < cls.highest_optimal_pH_microbes | ||
| < cls.max_pH_microbes | ||
| ): | ||
| raise ValueError("Microbe pH values not in increasing sequence") |
There was a problem hiding this comment.
Probably slightly easier to understand if these are called "thresholds" rather than "values" (apologies if this is just copied directly from something I previously wrote 🫠)
|
Hi @jacobcook1995, we're researchers from Stockholm University studying communication on GitHub. A message you posted may come across as rude, disrespectful, or unreasonable — something that could discourage others from contributing or sharing their ideas:
You can opt out of messages like this here. If you are the moderator of this repository, you can opt out of messages like this for the entire repo here. Note Participate in our four-question survey to provide feedback, and follow us for project updates! This message was flagged by an automated system and reviewed using an AI tool to check for tone and clarity concerns. We’re not here to judge. Our goal is to help make collaboration smoother for everyone. Messages like this can sometimes cause stress or burnout for others involved in the project. Here are a few quick tips to make messages clearer and easier to engage with:
Thanks for contributing to GitHub and helping improve collaboration for everyone! |
davidorme
deleted the
1108-move-soils-over-to-the-new-configuration-system
branch
|
Interesting. I guess it is a good thing to try and encourage polite discourse on GitHub - lack of moderation is a serious issue in online platforms. Having said that - what's the false positive rate like on what your tool flags? The context here is that @jacobcook1995 is commenting on his own code - without that context it's very difficult to triage comments, particularly such short ones. |
4 participants
Description
This PR:
soil.model_configto add docstrings and then move a bunch of validation (ordering of pH values, required enzyme and microbial group classes) out of the model methods and into field and model validators.find_microbial_stoichiometrieshelper function over to theanimal.cnpmodule, since it now has access to the full configuration directly and doesn't needsoilto look up the data for it.soil.microbial_groups.EnzymeConstantsand themake_full_set_of_enzymescompletely, since the old dataclass is functionally identical to the new validator classsoil.model_config.SoilEnzymeClassand the function is basically identical to the custom validation on that pydantic model. It's basically a straight swap of things that aredict[str, EnzymeConstants]fordict[str, SoilEnzymeClass].Note
There is similarly a huge amount of overlap between the new validator class
soil.model_config.SoilMicrobialGroupandsoil.microbial_groups.MicrobialGroupConstants, but that has an extra factory method. I'm sure we can combine the two, but maybe leave this for another day - lets get configuration up and running first.SoilConststoSoilConstantsetc. anyway.soil.model_configtests.Fixes #1108
Type of change
Key checklist
pre-commitchecks:$ pre-commit run -a$ poetry run pytestFurther checks